Update thirdparty tests, make subprocess testrunner run user code in root classloader #2561
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lihaoyi
changed the title
lihaoyi
changed the title
lefou
reviewed
May 31, 2023
Comment on lines
-143
to
-164
| private def copy( | ||
| framework: String = framework, | ||
| classpath: Seq[String] = classpath, | ||
| arguments: Seq[String] = arguments, | ||
| sysProps: Map[String, String] = sysProps, | ||
| outputPath: String = outputPath, | ||
| colored: Boolean = colored, | ||
| testCp: String = testCp, | ||
| homeStr: String = homeStr, | ||
| globSelectors: Seq[String] = globSelectors | ||
| ): TestArgs = new TestArgs( | ||
| framework, | ||
| classpath, | ||
| arguments, | ||
| sysProps, | ||
| outputPath, | ||
| colored, | ||
| testCp, | ||
| homeStr, | ||
| globSelectors | ||
| ) | ||
|
|
There was a problem hiding this comment.
We made this one private and some other tricks to keep the option to evolve this class in a binary compatible way. What's the intention of changing that?
There was a problem hiding this comment.
This is still WIP, but I want to try to adjust the code path so we don't need to use a case class here. That would make maintaining binary compatibility much simpler
|
Should be ready for review I think |
lihaoyi
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Move
thirdpartyfromintegration/toexample/, and simplify them down toacyclic,fansi, andjimfs. We can add more complex builds later if we want, but for now just having some realistic builds that are not bitrotted and old is already an improvement over what we have now.Flip the sub-process
TestRunnerclassloaders on its head: rather than runnerTestRunner.scalain the boot classloader and user code in a sub-classloader, we run user code in the boot classloader andTestRunner.scalain a sub classloader. This is necessary to make JimFS work, and would probably fix Tests fail due to class loading issue - GraalVM cannot be used. #716 and related issues.Broke up and cleaned up
TestRunner.scala:TestRunnerandTestRunnerMain0, the two entrypoints, fromTestRunnerUtilsTestModuleand read from the file byTestRunnerClosableIteratorwithgeny.Generator, which also provides guaranteed closing after iterationMarked most of
testrunneras@internal. Most of these could beprivate[mill], but I think we don't have a clear idea of how this stuff is meant to be used yet, and it's plausible that other people may want to interact with the testrunner in their own custom test suites. I think@internalis better suited for now until we either (a) know better what a stable API looks like or (b) decide that this stuff should not be used externally.We have much more control over
TestRunner.scalathan we do over user code, so we can make sure thatTestRunner.scalaworks when running in a semi-isolated classloader, whereas for user code we can offer no such guarantees. We can't do anything for.testLocalthat runs in-process, but at least for.testwe can try to offer them a clean classloader environment.The classloader for user code is not 100% clean: we still have a single class
mill.testrunner.entrypoint.TestRunnerMainadded within it. That's probably OK, since no matter what we do w.r.t. shading and stuff we still need somewhere to put our own main method to bootstrap themill.testrunnerclassloader.The testrunner changes are covered by existing unit and integration/example tests, and the new "user code runs in clean classloader" logic is covered by
example.thirdparty[jimfs].local.